-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix truncate(2) mtime and ctime handling #6819
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6819 +/- ##
=========================================
Coverage ? 75.26%
=========================================
Files ? 298
Lines ? 94476
Branches ? 0
=========================================
Hits ? 71109
Misses ? 23367
Partials ? 0
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just a couple of comments.
# | ||
# Verify $valuea is equal to $valueb, otherwise print a error | ||
# | ||
function log_must_eq # <valuea> <valueb> <name> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth moving log_must_eq
and log_must_neq
to libtest.shlib
? They seem generic enough to be placed there and I can think of at least a handful of test cases that could benefit from having such functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest logapi.shlib
if you want to stick with the log_must
prefix, otherwise math.shlib
and an alternate name. math.shlib
already contains a vaguely similar helper function called, within_percent
.
typeset -i name=$3 | ||
|
||
if [[ $valuea -eq $valueb ]]; then | ||
log_fail "Compared $name should not match: $valuea == $valueb" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Should make this message consistent with the log_must_eq
version. Compared $name should not be equal:...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what i did originally (copy/pasted the message from the other function), but then i noticed it doesn't fit in a single 80-cols line, and i don't like multiline strings.
I'll try changing variable names to fit 80 cols (with the appropriate message) when i refresh this PR.
pkgexecdir = $(datadir)/@PACKAGE@/zfs-tests/tests/functional/truncate | ||
|
||
pkgexec_PROGRAMS = truncate | ||
truncate_SOURCES = truncate.c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than introduce a new binary how about extending the existing tests/zfs-tests/cmd/file_trunc/file_trunc.c
command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it, but:
-
file_trunc
opens the target file withO_TRUNC
unconditionally, my fear was this would invalidate the test case (i actually just checked, it doesn't). -
file_trunc
unconditionally does awrite()
and then atruncate()
, i would have to add an additional flag to the CLI to avoid that firstwrite()
(again, this could invalidate the test case), and optionally update any consumer. -
do_trunc()
takes aint fd
param but we need achar *path
to testtruncate()
, so this may require some more code refactoring to work.
I'll see what i can do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more considerations:
-
Most of
file_trunc
consumers (online_offline_001_pos
,online_offline_003_neg
,replacement_001_pos
,replacement_002_pos
andreplacement_003_pos
) are exploiting the factfile_trunc -c 0
will generate a stream of writes to test zpool operations during IO: they should probably usefile_write
instead, which currently doesn't support-c 0
. -
The only other two consumers (
sparse_001_pos
andtruncate_001_pos
) are basically the same test, they both usefile_trunc
with the same parameters (-r
just reads the data just written and verify it) and then useverify_filesys
(fromlibtest.shlib
) to "verify" the filesystem; to be honest i don't understand what these tests are really doing.
root@linux:~# sysdig proc.name = 'file_trunc' and evt.type = execve -p%proc.cmdline
file_trunc -f 67108864 -b 512 -c 16384 -r /var/tmp/testdir/testfile.sparse
file_trunc -f 67108864 -b 512 -c 16384 /var/tmp/testdir/testfile.2802
All things considered the ZTS could really use some cleaning up here: once we do that we can probably drop do_write()
from file_trunc.c
and use it to properly test truncate(2)/ftruncate(2)
without the need to introduce a new truncate
binary.
Most of the work need is very much out of scope here though, and to avoid issues with OpenZFS patches should be also ported to Illumos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the interests of getting this fix merged and limiting the scope of the change let's go ahead with the PR pretty much as-is. The one change I'd like to see is to rename this new binary to avoid any confusion with the standard truncate(1)
command. Then we can follow up with any cleanup we decide is worthwhile.
# | ||
# Verify $valuea is equal to $valueb, otherwise print a error | ||
# | ||
function log_must_eq # <valuea> <valueb> <name> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest logapi.shlib
if you want to stick with the log_must
prefix, otherwise math.shlib
and an alternate name. math.shlib
already contains a vaguely similar helper function called, within_percent
.
pkgexecdir = $(datadir)/@PACKAGE@/zfs-tests/tests/functional/truncate | ||
|
||
pkgexec_PROGRAMS = truncate | ||
truncate_SOURCES = truncate.c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the interests of getting this fix merged and limiting the scope of the change let's go ahead with the PR pretty much as-is. The one change I'd like to see is to rename this new binary to avoid any confusion with the standard truncate(1)
command. Then we can follow up with any cleanup we decide is worthwhile.
On Linux, ftruncate(2) always changes the file timestamps, even if the file size is not changed. However, in case of a successfull truncate(2), the timestamps are updated only if the file size changes. This translates to the VFS calling the ZFS Posix Layer "setattr" function (zpl_setattr) with ATTR_MTIME and ATTR_CTIME unconditionally set on the iattr mask only when doing a ftruncate(2), while the truncate(2) is left to the filesystem implementation to be dealt with. This behaviour is consistent with POSIX:2004/SUSv3 specifications where there's no explicit requirement for file size changes to update the timestamps only for ftruncate(2): http://pubs.opengroup.org/onlinepubs/009695399/functions/truncate.html http://pubs.opengroup.org/onlinepubs/009695399/functions/ftruncate.html This has been later updated in POSIX:2008/SUSv4 where, for both truncate(2)/ftruncate(2), there's no mention of this size change requirement: http://austingroupbugs.net/view.php?id=489 http://pubs.opengroup.org/onlinepubs/9699919799/functions/truncate.html http://pubs.opengroup.org/onlinepubs/9699919799/functions/ftruncate.html Unfortunately the Linux VFS is still calling into the ZPL without ATTR_MTIME/ATTR_CTIME set in the truncate(2) case: we fix this by explicitly updating the timestamps when detecting the ATTR_SIZE bit, which is always set in do_truncate(), on the iattr mask. Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
On Linux, ftruncate(2) always changes the file timestamps, even if the file size is not changed. However, in case of a successfull truncate(2), the timestamps are updated only if the file size changes. This translates to the VFS calling the ZFS Posix Layer "setattr" function (zpl_setattr) with ATTR_MTIME and ATTR_CTIME unconditionally set on the iattr mask only when doing a ftruncate(2), while the truncate(2) is left to the filesystem implementation to be dealt with. This behaviour is consistent with POSIX:2004/SUSv3 specifications where there's no explicit requirement for file size changes to update the timestamps only for ftruncate(2): http://pubs.opengroup.org/onlinepubs/009695399/functions/truncate.html http://pubs.opengroup.org/onlinepubs/009695399/functions/ftruncate.html This has been later updated in POSIX:2008/SUSv4 where, for both truncate(2)/ftruncate(2), there's no mention of this size change requirement: http://austingroupbugs.net/view.php?id=489 http://pubs.opengroup.org/onlinepubs/9699919799/functions/truncate.html http://pubs.opengroup.org/onlinepubs/9699919799/functions/ftruncate.html Unfortunately the Linux VFS is still calling into the ZPL without ATTR_MTIME/ATTR_CTIME set in the truncate(2) case: we fix this by explicitly updating the timestamps when detecting the ATTR_SIZE bit, which is always set in do_truncate(), on the iattr mask. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: loli10K <ezomori.nozomu@gmail.com> Closes openzfs#6811 Closes openzfs#6819
On Linux, ftruncate(2) always changes the file timestamps, even if the file size is not changed. However, in case of a successfull truncate(2), the timestamps are updated only if the file size changes. This translates to the VFS calling the ZFS Posix Layer "setattr" function (zpl_setattr) with ATTR_MTIME and ATTR_CTIME unconditionally set on the iattr mask only when doing a ftruncate(2), while the truncate(2) is left to the filesystem implementation to be dealt with. This behaviour is consistent with POSIX:2004/SUSv3 specifications where there's no explicit requirement for file size changes to update the timestamps only for ftruncate(2): http://pubs.opengroup.org/onlinepubs/009695399/functions/truncate.html http://pubs.opengroup.org/onlinepubs/009695399/functions/ftruncate.html This has been later updated in POSIX:2008/SUSv4 where, for both truncate(2)/ftruncate(2), there's no mention of this size change requirement: http://austingroupbugs.net/view.php?id=489 http://pubs.opengroup.org/onlinepubs/9699919799/functions/truncate.html http://pubs.opengroup.org/onlinepubs/9699919799/functions/ftruncate.html Unfortunately the Linux VFS is still calling into the ZPL without ATTR_MTIME/ATTR_CTIME set in the truncate(2) case: we fix this by explicitly updating the timestamps when detecting the ATTR_SIZE bit, which is always set in do_truncate(), on the iattr mask. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: loli10K <ezomori.nozomu@gmail.com> Closes openzfs#6811 Closes openzfs#6819
On Linux, ftruncate(2) always changes the file timestamps, even if the file size is not changed. However, in case of a successfull truncate(2), the timestamps are updated only if the file size changes. This translates to the VFS calling the ZFS Posix Layer "setattr" function (zpl_setattr) with ATTR_MTIME and ATTR_CTIME unconditionally set on the iattr mask only when doing a ftruncate(2), while the truncate(2) is left to the filesystem implementation to be dealt with. This behaviour is consistent with POSIX:2004/SUSv3 specifications where there's no explicit requirement for file size changes to update the timestamps only for ftruncate(2): http://pubs.opengroup.org/onlinepubs/009695399/functions/truncate.html http://pubs.opengroup.org/onlinepubs/009695399/functions/ftruncate.html This has been later updated in POSIX:2008/SUSv4 where, for both truncate(2)/ftruncate(2), there's no mention of this size change requirement: http://austingroupbugs.net/view.php?id=489 http://pubs.opengroup.org/onlinepubs/9699919799/functions/truncate.html http://pubs.opengroup.org/onlinepubs/9699919799/functions/ftruncate.html Unfortunately the Linux VFS is still calling into the ZPL without ATTR_MTIME/ATTR_CTIME set in the truncate(2) case: we fix this by explicitly updating the timestamps when detecting the ATTR_SIZE bit, which is always set in do_truncate(), on the iattr mask. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: loli10K <ezomori.nozomu@gmail.com> Closes openzfs#6811 Closes openzfs#6819
On Linux, ftruncate(2) always changes the file timestamps, even if the file size is not changed. However, in case of a successfull truncate(2), the timestamps are updated only if the file size changes. This translates to the VFS calling the ZFS Posix Layer "setattr" function (zpl_setattr) with ATTR_MTIME and ATTR_CTIME unconditionally set on the iattr mask only when doing a ftruncate(2), while the truncate(2) is left to the filesystem implementation to be dealt with. This behaviour is consistent with POSIX:2004/SUSv3 specifications where there's no explicit requirement for file size changes to update the timestamps only for ftruncate(2): http://pubs.opengroup.org/onlinepubs/009695399/functions/truncate.html http://pubs.opengroup.org/onlinepubs/009695399/functions/ftruncate.html This has been later updated in POSIX:2008/SUSv4 where, for both truncate(2)/ftruncate(2), there's no mention of this size change requirement: http://austingroupbugs.net/view.php?id=489 http://pubs.opengroup.org/onlinepubs/9699919799/functions/truncate.html http://pubs.opengroup.org/onlinepubs/9699919799/functions/ftruncate.html Unfortunately the Linux VFS is still calling into the ZPL without ATTR_MTIME/ATTR_CTIME set in the truncate(2) case: we fix this by explicitly updating the timestamps when detecting the ATTR_SIZE bit, which is always set in do_truncate(), on the iattr mask. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: loli10K <ezomori.nozomu@gmail.com> Closes openzfs#6811 Closes openzfs#6819
Description
On Linux,
ftruncate(2)
always changes the file timestamps, even if thefile size is not changed. However, in case of a successfull
truncate(2)
, the timestamps are updated only if the file size changes.This translates to the VFS calling the ZFS Posix Layer "setattr"
function (
zpl_setattr
) withATTR_MTIME
andATTR_CTIME
unconditionallyset on the
iattr
mask only when doing aftruncate(2)
, while thetruncate(2)
is left to the filesystem implementation to be dealt with.This behaviour is consistent with POSIX:2004/SUSv3 specifications
where there's no explicit requirement for file size changes to update
the timestamps only for
ftruncate(2)
:http://pubs.opengroup.org/onlinepubs/009695399/functions/truncate.html
http://pubs.opengroup.org/onlinepubs/009695399/functions/ftruncate.html
This has been later updated in POSIX:2008/SUSv4 where, for both
truncate(2)
/ftruncate(2)
, there's no mention of this size changerequirement:
http://austingroupbugs.net/view.php?id=489
http://pubs.opengroup.org/onlinepubs/9699919799/functions/truncate.html
http://pubs.opengroup.org/onlinepubs/9699919799/functions/ftruncate.html
Unfortunately the Linux VFS is still calling into the ZPL without
ATTR_MTIME
/ATTR_CTIME
set in thetruncate(2)
case: we fix this byexplicitly updating the timestamps when detecting the
ATTR_SIZE
bit,which is always set in
do_truncate()
, on theiattr
mask.Motivation and Context
Fix #6811
How Has This Been Tested?
SystemTap and https://github.com/zfsonlinux/fstest.
Will add new code to the ZTS if/when this doesn't break any old test case.
Types of changes
Checklist:
Signed-off-by
.